Refactor unified_qr.rs to use bitcoin-payment-instructions #607

chuksys · 2025-08-11T18:17:26Z

This PR introduces a unified.rs module (which is a refactor of the unified_qr.rs module) - this refactor allows us to use this module as a single API for sending payments to BIP 21/321 URIs as well as BIP 353 HRNs, creating a simpler interface for users.

https://github.com/rust-bitcoin/bitcoin-payment-instructions is used to parse BIP 21/321 URIs as well as the BIP 353 HRNs.

Changes

The unified_qr.rs module has been renamed to unified.rs.
The UnifiedQrPayment struct has been renamed to UnifiedPayment.
The QRPaymentResult enum has been renamed to PaymentResult.
The send method in unified.rs now supports sending to both BIP 21/321 URIs as well as BIP 353 HRNs.
The integration tests for the module have been updated accordingly.

I will follow-up with adding an end-to-end test that asserts that sending to HRNs using this API works.

This PR closes #521

This rename reflects that this module is a unified payment interface for both QR code payments and HRN payments passed in as a string without scanning a QR code

…PaymentResult These renamings are necessary to reflect the expanded responsibilities for this module.

This commit adds a HRN Resolver to the Node struct which will be useful for resolving HRNs when making BIP 353 payments

…353 HRNs

This commit refactors the integration tests for unified_payment - specifically qr_uri (bip21) generation and sending to qr_uri (bip21)

ldk-reviews-bot · 2025-08-11T18:17:29Z

I've assigned @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull

Cool, thanks! Did a first ~high-level round. Feel free to undraft this.

tnull · 2025-08-14T08:28:16Z

src/payment/unified.rs

@@ -304,7 +304,7 @@ impl DeserializationError for Extras {
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::payment::unified_qr::Extras;
+	use crate::payment::unified::Extras;


nit: I think we can just drop this line as the super::* import above covers Extras. Although, it would actually be preferable to switch to explicit import here, i.e., use super::{Extras, ...}.

tnull · 2025-08-14T08:29:39Z

src/lib.rs

@@ -952,8 +952,8 @@ impl Node {
 	/// [BOLT 12]: https://github.com/lightning/bolts/blob/master/12-offer-encoding.md
 	/// [BIP 21]: https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki
 	#[cfg(feature = "uniffi")]
-	pub fn unified_qr_payment(&self) -> Arc<UnifiedQrPayment> {


Mind updating the docs here and above that this will now also be used for HRNs?

tnull · 2025-08-14T08:30:00Z

src/payment/unified.rs

@@ -189,7 +189,7 @@ impl UnifiedQrPayment {
 /// [`PaymentId`]: lightning::ln::channelmanager::PaymentId
 /// [`Txid`]: bitcoin::hash_types::Txid
 #[derive(Debug)]
-pub enum QrPaymentResult {
+pub enum PaymentResult {


Maybe UnifiedPaymentResult then?

tnull · 2025-08-14T08:31:21Z

Cargo.toml

@@ -91,6 +91,7 @@ log = { version = "0.4.22", default-features = false, features = ["std"]}

 vss-client = "0.3"
 prost = { version = "0.11.6", default-features = false}
+bitcoin-payment-instructions = { git = "https://github.com/rust-bitcoin/bitcoin-payment-instructions" }


Ah, @TheBlueMatt, can we get a bitcoin-payment-instructions release that includes rust-bitcoin/bitcoin-payment-instructions#6 ?

@chuksys In the meantime, please import a specific commit via rev = "..", as otherwise our builds might break as soon as bitcoin-payment-instructions's main changes.

tnull · 2025-08-14T08:34:20Z

src/builder.rs

@@ -1633,6 +1635,14 @@ fn build_with_store_internal(
 	let (stop_sender, _) = tokio::sync::watch::channel(());
 	let background_processor_task = Mutex::new(None);

+	let hrn_resolver = Arc::new(LDKOnionMessageDNSSECHrnResolver::new(network_graph.clone()));


nit: Please use Arc::clone(&..) here and everywhere for Arcs. This helps distinguishing that we in fact do not clone the actual value here.

tnull · 2025-08-14T08:38:29Z

src/payment/unified.rs

+	amount::Amount as BPIAmount, PaymentInstructions, PaymentMethod,
+};
+
+use crate::types::HRNResolver;


nit: Let's move this up to the other crate:: imports.

tnull · 2025-08-14T08:54:33Z

tests/integration_tests_rust.rs

Please include the test changes in the corresponding commit(s) that require them. Each commit should build and test individually, not just the whole PR.

tnull · 2025-08-14T08:58:31Z

src/payment/unified.rs

-				Ok(payment_id) => return Ok(QrPaymentResult::Bolt12 { payment_id }),
-				Err(e) => log_error!(self.logger, "Failed to send BOLT12 offer: {:?}. This is part of a unified QR code payment. Falling back to the BOLT11 invoice.", e),
+	/// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki
+	pub async fn send(


So this is the first async method (mod Node::next_event_async) we do expose in the API, which makes it a bit incongruent with the rest of the API.

That said, I'm not super opposed to it, let's use this as a first experimental venture into async-API world. If any users complain, we could reuse our runtime to drive this and expose a blocking API, which is what we did so far.

tnull · 2025-08-14T08:59:32Z

src/payment/unified.rs

+	pub async fn send(
+		&self, uri_str: &str, amount_msat: Option<u64>,
+	) -> Result<PaymentResult, Error> {
+		let instructions = match PaymentInstructions::parse(


Rather than matching, let's just use .map_err(|e| .. )?, which should make this more readable and less vertical.

Also possible for ~most other matches below.

tnull · 2025-08-14T09:03:40Z

src/payment/unified.rs

+					return Err(Error::InvalidAmount);
+				}
+			},
+			PaymentInstructions::FixedAmount(ref instr) => instr.clone(),


Should we at least check that the amount_msat (if provided) is greater or equal to the fixed amount?

tnull

This needs a rebase by now, also.

chuksys added 7 commits August 11, 2025 18:53

Rename unified_qr to unified

8e190ee

This rename reflects that this module is a unified payment interface for both QR code payments and HRN payments passed in as a string without scanning a QR code

rename UnifiedQRPayment to UnifiedPayment, rename QRPaymentResult to …

1903144

…PaymentResult These renamings are necessary to reflect the expanded responsibilities for this module.

add bitcoin-payment-instructions to cargo.toml

7961b8f

Add hrn_resolver to Node

6ff4e41

This commit adds a HRN Resolver to the Node struct which will be useful for resolving HRNs when making BIP 353 payments

Pass hrn_resolver into UnifiedPayment and update unified.rs docs

0d7fc1c

Refactor unified.rs to support sending to BIP 21 URIs as well as BIP …

8c78b5d

…353 HRNs

Refactor unified_payment tests

7909c0f

This commit refactors the integration tests for unified_payment - specifically qr_uri (bip21) generation and sending to qr_uri (bip21)

chuksys mentioned this pull request Aug 11, 2025

Add support for sending to human-readable names (BIP 353) #528

Closed

chuksys marked this pull request as draft August 11, 2025 18:25

tnull self-requested a review August 13, 2025 07:57

tnull reviewed Aug 14, 2025

View reviewed changes

chuksys marked this pull request as ready for review August 17, 2025 18:00

ldk-reviews-bot requested a review from joostjager August 17, 2025 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor unified_qr.rs to use bitcoin-payment-instructions #607

Refactor unified_qr.rs to use bitcoin-payment-instructions #607

chuksys commented Aug 11, 2025

Uh oh!

ldk-reviews-bot commented Aug 11, 2025 •

edited

Loading

Uh oh!

tnull left a comment

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull Aug 14, 2025 •

edited

Loading

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull Aug 14, 2025

Uh oh!

tnull left a comment

Uh oh!

Uh oh!

Refactor unified_qr.rs to use bitcoin-payment-instructions #607

Are you sure you want to change the base?

Refactor unified_qr.rs to use bitcoin-payment-instructions #607

Conversation

chuksys commented Aug 11, 2025

Changes

Uh oh!

ldk-reviews-bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Aug 11, 2025 •

edited

Loading

tnull Aug 14, 2025 •

edited

Loading